[Test] Don't include '*' in glob pattern literals#133114
[Test] Don't include '*' in glob pattern literals#133114tvernum merged 1 commit intoelastic:mainfrom
Conversation
This changes the random patterns that are generated inside `GlobTests` to not generate `*` characters when a literal string is intended
tvernum
added
:Core/Infra/REST API
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| assertNonMatch("123*", "abc123def"); | ||
|
|
||
| var prefix = randomAsciiString(randomIntBetween(2, 12)); | ||
| var prefix = randomAsciiStringNoAsterisks(randomIntBetween(2, 12)); |
There was a problem hiding this comment.
This was testing (potentially) things with multiple * before, like a "prefix" 123*45* (and I think the assertions were correct too?); are we sure this is not something we want to test?
There was a problem hiding this comment.
If prefix was *a then pattern would be *a*
That works (logically) for the matches
*a*matches*a*a*matches*a+ random
But it breaks the logic for the non-matches
For example:
assertNonMatch(pattern, prefix.substring(1));
That would test ("*a*" , "a") which would match, when it is not supposed to.
The intent of that test is that prefix is a literal (e.g. ab) so that ab* does not match b
There was a problem hiding this comment.
Ah I see, there are randoms for which the test would fail.
Seems fair, if we feel the need for a test with multiple * we can add it explicitly, and be more precise in these prefix/suffix tests.
There was a problem hiding this comment.
Why is the range in these tests randomIntBetween(2, 12)? That seems an odd choice that explicitly avoids an important corner case: shouldn't 1 also work? And, what's the benefit from testing lengths greater than 2? I think we ought to use something more like randomIntBetween(1,2) here.
| assertNonMatch("123*", "abc123def"); | ||
|
|
||
| var prefix = randomAsciiString(randomIntBetween(2, 12)); | ||
| var prefix = randomAsciiStringNoAsterisks(randomIntBetween(2, 12)); |
There was a problem hiding this comment.
Ah I see, there are randoms for which the test would fail.
Seems fair, if we feel the need for a test with multiple * we can add it explicitly, and be more precise in these prefix/suffix tests.
tvernum
added
the
auto-backport
💔 Backport failed
You can use sqren/backport to manually backport by running |
This changes the random patterns that are generated inside `GlobTests` to not generate `*` characters when a literal string is intended
This changes the random patterns that are generated inside `GlobTests` to not generate `*` characters when a literal string is intended
This changes the random patterns that are generated inside `GlobTests` to not generate `*` characters when a literal string is intended
…lastic#133125) This changes the random patterns that are generated inside `GlobTests` to not generate `*` characters when a literal string is intended
Umute test fixed in elastic#133114
Unmute tests fixed in elastic#133114
* Make Glob non-recursive (#132798) This changes the implementation of `Glob` (used by `FilterPath`) to use a non-recursive algorithm for improved efficiency and stability * [Test] Don't include '*' in glob pattern literals (#133114) (#133125) This changes the random patterns that are generated inside `GlobTests` to not generate `*` characters when a literal string is intended
|
@JVerwolf - confirmed, it does fix all those tests. |
5 participants
This changes the random patterns that are generated inside
GlobTeststo not generate*characters when a literal string is intended